refactor: Fix header styling issues - #1292
Conversation
| z-index: 101; | ||
|
|
||
| svg { | ||
| filter: invert(40%) sepia(0%) saturate(3773%) hue-rotate(315deg) | ||
| brightness(73%) contrast(81%); | ||
| } |
There was a problem hiding this comment.
The z-index is entirely unused, goes back to the repo creation. This style would only apply to the 'About' or i18n dropdown buttons which wouldn't need a z-index as they're kept inline, exactly where they were.
The SVG filter is a far more complex method of updating the icon color; way easier to just switch the icon to use currentColor and piggyback off the color setting just a few lines up (color: #555).
rschristian
changed the title
| fill: rgba(255, 255, 255, 0.6); | ||
| cursor: pointer; | ||
| z-index: 560; | ||
| z-index: 460; |
There was a problem hiding this comment.
I think this was a typo.
The slide-out sidebar has a z-index of 450, and the slide-down header has a z-index of 500. As such, when the header is expanded, there's really no reason for the sidebar toggle button to be displayed -- you're toggling something that can't be seen.
z-index of 460 allows it to still sit on top of the sidebar when expanded but hidden behind the header if it is expanded.
| height: var(--vh); | ||
| height: calc(var(--vh) - var(--banner-height)); |
There was a problem hiding this comment.
Predates my involvement, but IIUC, this was a problem introduced when the banner was added. It seems like the idea was that the social icons would be at the bottom of the screen when the togglable header was expanded, however, it never took into account the banner height. As such, the icons were always clipped off the screen.
The handful of white pixels at the very bottom of the screen are our icons, but you obviously can't interact with them as-is.
| position: relative; | ||
| display: inline-block; | ||
| font-size: 100%; | ||
| user-select: none; | ||
| -webkit-user-drag: none; |
There was a problem hiding this comment.
AFAICT, completely unnecessary.
| font-size: 100%; | ||
| user-select: none; | ||
| -webkit-user-drag: none; | ||
| display: flex; |
There was a problem hiding this comment.
Fixes the issue of header items wrapping on some screensizes:
Still not perfect, there's now some overlapping in the header, but wrapping is very bad.
| display: flex; | ||
| align-items: center; |
There was a problem hiding this comment.
Ensures the little dropdown arrow next to 'About' (or any future dropdown pages) is vertically centered:
| touch-action: none; | ||
|
|
There was a problem hiding this comment.
When the header is opened, we should not let the user scroll the page in the background.
| top: calc(var(--vh) - 3rem); /* TODO: Why does bottom not work? */ | ||
| display: flex; | ||
| display: none; | ||
| bottom: calc(env(safe-area-inset-bottom) + 1rem); |
There was a problem hiding this comment.
Hides the icons by default and uses the more straightforward bottom now that our parent height has been corrected (see comment above).
What it looks like now:
| padding: 18px 23px 12px; | ||
| line-height: var(--header-height); | ||
| text-align: center; | ||
| z-index: 100; |
There was a problem hiding this comment.
Superfluous z-index, it sits on the header (z-index of 500 already) and nothing overlaps it either.
marvinhagemeister
left a comment
marvinhagemeister
left a comment
There was a problem hiding this comment.
All great fixes! Quality work as usual 👍
2 participants
Will review & explain the changes one-by-one below, but looks to fix a handful of issues that have crept up. None are too problematic but good opportunity to review & fix some things.